Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix symbol rendering for multipoints #3763

Merged
merged 3 commits into from
Dec 13, 2016
Merged

Fix symbol rendering for multipoints #3763

merged 3 commits into from
Dec 13, 2016

Conversation

lucaswoj
Copy link
Contributor

@lucaswoj lucaswoj commented Dec 8, 2016

Via a support request, @mapsam and I discovered that we do not render any symbols for multipoint geometries. This is inconsistent with our behaviour for circles in which we do render a circle at each point in the multipoint geometry (see #2027).

This PR tries to differentiate multipoint geometries from polygons and perform the correct label placement in each case.

cc @mourner @flippmoke @mapsam @jfirebaugh @colleenmcginnis

Open Questions

  • Is there a better way to differentiate between a multipoint and a polygon within GL JS?
  • Should we have a different symbol-placement modes for point, line, and polygon / centroid?
  • Are there other cases in which we don't handle multipoints properly?

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • manually test the debug page

mourner
mourner reviewed Dec 8, 2016
View reviewed changes
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rely on feature.type for discerning between points and polygons?

@lucaswoj lucaswoj force-pushed the multipoint-symbol-placement branch from ecb4af5 to 8812ad4 Compare December 9, 2016 00:31
@lucaswoj
Copy link
Contributor Author

lucaswoj commented Dec 9, 2016

Updated with new tests & use of the vector tile type field 😎

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Dec 9, 2016

It's unfortunate that we place circles & symbols differently for lines, polygons, and multi-things. If pole-of-inaccessability symbol placement were a symbol-placement mode, we could have more consistent rendering.

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Dec 9, 2016

PR for new tests @ mapbox/mapbox-gl-test-suite#192

@lucaswoj lucaswoj merged commit 32fcbfe into master Dec 13, 2016
@lucaswoj lucaswoj deleted the multipoint-symbol-placement branch December 13, 2016 20:03
@jfirebaugh jfirebaugh mentioned this pull request Dec 13, 2016
Closed
jfirebaugh added a commit to mapbox/mapbox-gl-native that referenced this pull request Dec 15, 2016
@jfirebaugh
[core] Fix symbol rendering for multipoints
da7486b 
Ports mapbox/mapbox-gl-js#3763 and mapbox/mapbox-gl-js#3806.
jfirebaugh added a commit to mapbox/mapbox-gl-native that referenced this pull request Dec 20, 2016
@jfirebaugh
[core] Fix symbol rendering for multipoints
dad57df 
Ports mapbox/mapbox-gl-js#3763 and mapbox/mapbox-gl-js#3806.
@jfirebaugh jfirebaugh mentioned this pull request Mar 3, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mourner mourner mourner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lucaswoj @mourner